Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nohoist for cozy-ui #2605

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Add nohoist for cozy-ui #2605

merged 2 commits into from
Nov 7, 2024

Conversation

zatteo
Copy link
Contributor

@zatteo zatteo commented Nov 5, 2024

Avec ce commit, cozy-ui ne sera PLUS présent dans les node_modules à la racine. Il sera dans chaque node_modules de chaque package.

@zatteo zatteo requested a review from acezard as a code owner November 5, 2024 16:59
@zatteo zatteo changed the title Fix/test hoisting workspace Add nohoist for cozy-ui Nov 5, 2024
We got issues during a test from cozy-viewer using cozy-client
useClient because :
- PdfMobileViewer from cozy-viewer calls useClient/withClient from
cozy-libs/packages/cozy-viewer/node-modules/cozy-client
- FileImageLoader from cozy-ui calls  useClient/withClient from
cozy-libs/node-modules/cozy-ui/node_modules/cozy-client

So the client was not accessible from FileImageLoader leading to an
error during tests.

We fix this by using the nohoist feature  for cozy-ui.

See https://classic.yarnpkg.com/blog/2018/02/15/nohoist/
@zatteo zatteo force-pushed the fix/test-hoisting-workspace branch from 312a879 to 33c8565 Compare November 6, 2024 08:19
@JF-Cozy
Copy link
Contributor

JF-Cozy commented Nov 6, 2024

est-ce qu'on ne mettrait pas toutes les dep cozy (en tout cas celles qui ne sont pas dans cozy-libs) en nohoist ? 🤔

@zatteo
Copy link
Contributor Author

zatteo commented Nov 6, 2024

est-ce qu'on ne mettrait pas toutes les dep cozy (en tout cas celles qui ne sont pas dans cozy-libs) en nohoist ? 🤔

Je pense qu'il faut éviter de tout mettre en nohoist par défaut, nohoist c'est un peu désactiver le fonctionnement classique des workspaces, donc c'est un peu la roue de secours mais qui ralenti le fonctionnement général du monorepo en dupliquant les deps.

À mon avis :

  • à court terme si on a besoin de mettre d'autres deps là dedans, on hésite pas à les mettre
  • à long terme, est-ce que c'est normal de devoir faire ça ? Est-ce qu'il faudrait pas corriger quelques choses dans nos libs pour qu'elles supportent le monorepo correctement

Au final avec nohoist, c'est comme si un package du monorepo était comme une app Cozy par rapport à cozy-ui.

@JF-Cozy
Copy link
Contributor

JF-Cozy commented Nov 6, 2024

Avant on avait cozy-libs/node-modules/cozy-ui/node_modules/cozy-client et maintenant cozy-libs/packages/cozy-viewer/node-modules/cozy-ui n'a plus cozy-client dans ses node-modules ça utilise cozy-client directement depuis cozy-libs/packages/cozy-viewer/node-modules/cozy-client

Copy link
Contributor

@cballevre cballevre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça me parait la meilleur solution pour garder les tests fonctionnels. Je suis d'avis de limiter les nohoist au cas problématique pour l'instant 👍

@zatteo zatteo merged commit f50eb6d into master Nov 7, 2024
2 checks passed
@zatteo zatteo deleted the fix/test-hoisting-workspace branch November 7, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants